[opentitantool] Add support for Google TPM "ready signal" #21289
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jesultra
added
SW:opentitantool
CherryPick:earlgrey_es_sival
jesultra
force-pushed
the
tpm_ready
branch
3 times, most recently
from
ad2cc95 to
b392482
Compare
cfrantz
reviewed
Feb 12, 2024
| let Some((gsc_ready_pin, monitoring)) = gsc_ready_pin else { | ||
| return Ok(()); | ||
| }; | ||
| let start_time = SystemTime::now(); |
There was a problem hiding this comment.
Should we use std::time::{Instant,Duration} instead of SystemTime?
There was a problem hiding this comment.
I was wondering what kind of time call would not be affected by "ntpdate" or similar. Really, I do not care about absolute Instant, as long as I get two things that I can substract to get a Duration. Some sort of performance timer would be best, but I do not know what interface exist for that.
There was a problem hiding this comment.
After reading the documentation of std::time::Instant it appears that it exactly fits the bill. I had initially confused myself by assuming that Instant shared characteristics with the Java class of the same name.
| while { | ||
| self.spi | ||
| .run_transaction(&mut [spi::Transfer::Read(&mut buffer[0..1])])?; | ||
| buffer[0] & 1 == 0 | ||
| } { | ||
| retries -= 1; | ||
| if retries == 0 { | ||
| if SystemTime::now().duration_since(start_time)?.cmp(&TIMEOUT) == Ordering::Greater |
There was a problem hiding this comment.
Duration implements PartialOrd, which means you should be able to use the arithmetic operators here:
if SystemTime::now().duration_since(start_time)? > TIMEOUT { ... }
cfrantz
approved these changes
Feb 13, 2024
| @@ -233,20 +264,34 @@ impl SpiDriver { | |||
| self.spi | |||
| .run_transaction(&mut [spi::Transfer::Both(&req, &mut buffer)])?; | |||
| if buffer[3] & 1 == 0 { | |||
There was a problem hiding this comment.
Maybe leave some comments and/or create consts that explain what is happening with buffer here?
Prepare for adding support for GSC ready signal by refactoring some functions, enabling the next CL to more clearly show the functionality added. The only functional change in this CL is setting a timeout of 500 miliseconds, rather than 10 retries. The latter depends on debugger latency in an unpredictable way. Change-Id: I40c0d33c78dfd491842b4e3867a10d8a78ce45f5 Signed-off-by: Jes B. Klinke <[email protected]>
Google security chips deviate from the TPM standard. Because the chips
are not able to meet the requirements for response/processing time, they
employ an additional "ready" signal, generated by the GSC to indicate
when it has finished processing the most recent request, and is ready to
receive a new request.
This CL adds an optional flag `--gsc_ready` to `opentitantool {spi,i2c}
tpm` and to `tpm2_test_server`. Omitting this flag will result in
`opentitantool` use the standard TPM protocol, specifying the flag will
make `opentitantool` wait for the Google-specific ready signal at each
transaction.
Signed-off-by: Jes B. Klinke <[email protected]>
Change-Id: I1fa442095cb85c3073500f66fdb38c5547b038d9
|
Successfully created backport PR for |
jesultra
added a commit
to jesultra/opentitan
that referenced
this pull request
Feb 14, 2024
Is seems that PR lowRISC#21289 introduced a bug that prevents `opentitantool i2c tpm ...` from working at all. This CL restores the proper use of `Any<>` to allow passing the I2C driver object to the TPM command handler in the way that it expects. Change-Id: I2b791de25666876c49dccccd83bdbc9369e608be Signed-off-by: Jes B. Klinke <[email protected]>
jesultra
added a commit
to jesultra/opentitan
that referenced
this pull request
Feb 14, 2024
Is seems that PR lowRISC#21289 introduced a bug that prevents `opentitantool i2c tpm ...` from working at all. This CL restores the proper use of `Any<>` to allow passing the I2C driver object to the TPM command handler in the way that it expects. Also, a logic condition was accidentally reversed, making the new --gsc-ready flag ineffective for I2C. Change-Id: I2b791de25666876c49dccccd83bdbc9369e608be Signed-off-by: Jes B. Klinke <[email protected]>
jesultra
added a commit
that referenced
this pull request
Feb 15, 2024
Is seems that PR #21289 introduced a bug that prevents `opentitantool i2c tpm ...` from working at all. This CL restores the proper use of `Any<>` to allow passing the I2C driver object to the TPM command handler in the way that it expects. Also, a logic condition was accidentally reversed, making the new --gsc-ready flag ineffective for I2C. Change-Id: I2b791de25666876c49dccccd83bdbc9369e608be Signed-off-by: Jes B. Klinke <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Google security chips deviate from the TPM standard. Because the chips are not able to meet the requirements for response/processing time, they employ an additional "ready" signal, generated by the GSC to indicate when it has finished processing the most recent request, and is ready to receive a new request.
This CL adds an optional flag
--gsc_readytoopentitantool {spi,i2c} tpmand totpm2_test_server. Omitting this flag will result inopentitantooluse the standard TPM protocol, specifying the flag will makeopentitantoolwait for the Google-specific ready signal at each transaction.It is worth noting that the existing TPM driver for I2C is not implementing the protocol specified by TCG, but instead a Google-specific protocol, as the first use of I2C by legacy Titan GSC chips predates the TCG standard for I2C. In the future, we ought to enhance the driver to use the new standard I2C protocol, when
--gsc_readyis not provided.